Support Go 1.13 error chains in Cause
#215
Conversation
jayschwa
force-pushed
the
go1.13-compat
branch
from
c146378 to
c0e4a33
Compare
cause_go113.go
Outdated
| } | ||
|
|
||
| for err != nil { | ||
| if cause, ok := err.(causer); ok { |
There was a problem hiding this comment.
We should probably switch this to a type switch now:
switch v := err.(type) {
case causer:
err = v.Cause()
case unwrapper:
cause := errors.Unwrap(err)
if cause == nil {
return err
}
err = cause
default:
return err
}
| "testing" | ||
| ) | ||
|
|
||
| func TestErrorChainCompat(t *testing.T) { | ||
| func TestCauseErrorChainCompat(t *testing.T) { |
There was a problem hiding this comment.
I would also recommend also testing a WithMessage(fmt.Errorf("wrap2: %w", WithMessage(err, "wrap1"))), "wrap3")
cause_go113.go
Outdated
| // } | ||
| // | ||
| // If the error does not implement the Cause interface, the Cause | ||
| // function will fallback to the standard library's errors.Unwrap |
There was a problem hiding this comment.
Let’s be more explicitly clear about the contract we are building: will we return the original error of any arbitrary ordering of Cause and Unwrap? The text presented here is potentially ambiguous… hm, I think the original was somewhat ambiguous as well.
We should be clear that this function will keep working recursively until it finds an error that is both not a causer and would return errors.Unwrap(err) == nil.
There was a problem hiding this comment.
If an error has both Cause() and Unwrap(), I think Cause() should take precedence since that matches the original behavior of errors.Cause.
There was a problem hiding this comment.
That’s not what I am saying. If we have a causer wrapped by a fmt.Errorf wrapped error, which wraps a causer, what is the behavior?
The code as you have written would unwrap all layers (which I think it should), but we should document that it will “recursively” try unwrapping all layers until it gets an unwrapped error.
|
@jayschwa If I'm not wrong you're agree with the changes propose for @puellanivis so, could you do this changes, to try unblock this PR, thanks :) |
Imagine module A imports module B and both use `pkg/errors`. A uses
`errors.Cause` to inspect wrapped errors returned from B. As-is, B
cannot migrate from `errors.Wrap` to `fmt.Errorf("%w", err)` because
that would break `errors.Cause` calls in A. With this change merged,
`errors.Cause` becomes forwards-compatible with Go 1.13 error chains.
Module B will be free to switch to `fmt.Errorf("%w", err)` and that will
not break module A (so long as the top-level project pulls in the newer
version of `pkg/errors`).
jayschwa
force-pushed
the
go1.13-compat
branch
from
c0e4a33 to
c4261b0
Compare
|
Pushed some changes:
|
Imagine module A imports module B and both use
pkg/errors. A useserrors.Causeto inspect wrapped errors returned from B. As-is, Bcannot migrate from
errors.Wraptofmt.Errorf("%w", err)becausethat would break
errors.Causecalls in A. With this change merged,errors.Causebecomes forwards-compatible with Go 1.13 error chains.Module B will be free to switch to
fmt.Errorf("%w", err)and that willnot break module A (so long as the top-level project pulls in the newer
version of
pkg/errors).